Database support for delegating IP Pools for Oxide internal use #9103

bnaecker · 2025-09-26T18:22:05Z

Add database queries for marking any number of IP Pools as delegated for Oxide internal use, rather than assuming there's just one.
Rework queries for linking / unlinking IP Pool and Silo, making them concurrency-safe and preventing deletion when there are IPs or we're trying to delete the last internal IP Pool.
The public API does not change here. We're still assuming exactly one IP Pool per IP version for internal usage, and not allowing the link or pool to appear in the API. That'll be a follow-up commit.
Fixes Enforce relationship between IP Pools reserved for Oxide services silo #8948

- Add database queries for linking any number of IP Pools to the Oxide internal Silo, rather than assuming there's just one. - Rework queries for linking / unlinking IP Pool and Silo, making them concurrency-safe and preventing deletion when there are IPs or we're trying to delete the last internal IP Pool. - The public API does not change here. We're still assuming exactly one IP Pool per IP version for internal usage, and not allowing the link or pool to appear in the API. That'll be a follow-up commit. - Fixes #8948

bnaecker · 2025-10-01T16:34:27Z

I'm actually rethinking this a bit. There's a lot of complexity here, mostly because we use the ip_pool_resource table to link both customer silos, and to reflect the fact that a pool is "delegated" for our internal usage. The latter is when we link the pool to our hidden internal silo.

This makes the database queries pretty complicated, since we're trying to enforce that a pool is one of:

unlinked
linked to 1 or more customer silos
linked to our internal silo, of which there is exactly one

This also means you can't just look at the ip_pool table to know whether a pool is delegated, you have to join it with the resource table first.

I think I want to restructure this. I want to add an is_delegated column on the ip_pool table, which takes the place of the link between the pool and our internal silo. The ip_pool_resource table would only represent links to customer resources. This should make the conditional queries that enforce the above invariants simpler, and will also make #8947 easier, since we can tell if a pool is delegated from just one table.

All that to say, I'm moving this to a draft now, and I'll un-draftify it when I'm happy again.

david-crespo · 2025-10-01T16:58:27Z

So would a delegated pool be disallowed from being linked? Or could a pool do both?

bnaecker · 2025-10-01T17:09:14Z

That would be disallowed, just like it is today and in this PR. A pool is delegated to Oxide XOR linkable to customer silos.

- Add an `is_delegated` column to the `ip_pool` table - Enforce that pools are delegated XOR linkable to customer silos by looking at this table and using conditional update queries.

bnaecker · 2025-10-03T18:28:05Z

Alright, I've marked this ready for review now. The latest commits add an is_delegated column to the ip_pool table. This is true if the pool is delegated to Oxide, or false if an operator wants to revoke it. This takes the place of linking to the private, internal Oxide silo, meaning the ip_pool_resource table is just for linking to customer silos. This is a bit simpler than the previous code, especially for doing things like "listing all the delegated pools". But it's not as big a simplification as I'd hoped, so I'm open to reverting it to 44a2d02 if we want to link the internal Silo the same way.

…ip-pool-delegation

jgallagher · 2025-10-06T15:40:02Z

nexus/db-model/src/ip_pool.rs

    pub rcgen: i64,
+
+    /// True if the IP Pool has been delegated for Oxide use.
+    pub is_delegated: bool,


Coming at this review without much context, I'm wondering if is_delegated is too vague/generic and should say what it's delegated to, although I don't know how to be more specific without making it a lot longer. is_delegated_to_services? is_delegated_to_oxide? Entirely possible this is fine as-is and "look at the doc comment to see what it's delegated to", especially if we don't think we'll ever have other kinds of IP pool delegation.

@jgallagher and I talked a bit live about this. I think we'll change the name here to something like purpose or intended_use, and make this an enum rather than a bool. We'd have two variants today, indicating the pool can be used for external silos or for Oxide internal use. We can add variants if we need in the future for expressing things like "use by Nexus" or "use by external DNS".

Updated all this in e4ff3b8.

nexus/db-queries/src/db/datastore/ip_pool.rs

jgallagher · 2025-10-06T15:52:38Z

nexus/db-queries/src/db/datastore/ip_pool.rs

-    // NOTE: It'd be better to do one roundtrip to the DB, but this is
-    // rarely-used right now. We also want to return the authz and database
-    // objects, so we need the lookup-path mechanism.
+    // TODO-remove: Use list_ip_pools_for_internal instead.


Does this need to be addressed on this PR? Will callers of this method either fail (if the original built-in pools no longer exist) or succeed but with incorrect/incomplete information (if additional pools have been delegated)?

I think it's OK as-is, because you cannot actually change these pools yet. There's no API in Nexus for doing that. I have a follow-up PR in the works which makes the delegation API public, which also changes how this method works.

nexus/db-queries/src/db/datastore/ip_pool.rs

jgallagher · 2025-10-06T16:10:28Z

nexus/db-queries/src/db/datastore/ip_pool.rs

+            .await
+            .unwrap();
+
+        // We should be able to delete one of these.


This allows the delegated pools to be down to only IPv6 - do we know that works throughout the rest of the control plane? (E.g., I don't think reconfigurator cares, but AFAIK we've never tested an IPv6 only setup.)

I think the same comment as above applies -- this tests the internal database logic, but you can't exercise this through an API yet. I don't think it's possible to get into a situation in a real deployment where there is only an IPv6 pool. Even if there were a way to delete the IPv4 pool, I think that'd fail because there are addresses allocated out of it anyway.

- Add an enum for IP Pool reservation type, currently just representing internal or external uses. - Rework queries to be phrased in terms of "reserving" a pool for a specific use and checking for valid reservation types at query time. - Add expectorate tests for all complicated queries.

jgallagher

Changes look good; I like the enum over the boolean quite a lot. Mostly just have more vague questions for you. 😅

nexus/db-model/src/ip_pool.rs

jgallagher · 2025-10-08T14:40:48Z

nexus/db-queries/tests/output/reserve_internal_ip_pool.sql

+    )
+  AND CAST(
+      IF(
+        (SELECT count(1) FROM ip_pool WHERE time_deleted IS NULL AND reservation_type = $5 LIMIT 2)


Just thinking out loud - I think in practice this extra guard is probably not necessary? The guard above that we can't change the type of a pool if there are any services using IPs from it will prevent deleting any in-use pool, and there will always be at least one in-use pool (otherwise we wouldn't have any services with external IPs at all), right? But this seems super cheap and maybe makes it very clear that we always have to have at least one, so I'm on board with keeping it anyway.

I think the checks are doing slightly different things. The first one ensures that we don't delete the pool if there are addresses used from it. This one checks that there are at least two pools of this same reservation type. Those could be different in a future where we make more fine-grained distinctions between the types of internal pools, like NTP, DNS, etc. It might still be the case that the address-use guard is sufficient, but I'm with you that this is cheap and conservative :)

jgallagher · 2025-10-08T14:42:40Z

schema/crdb/dbinit.sql

-        NOT (
-            resource_type = 'silo' AND
-            resource_id = '001de000-5110-4000-8000-000000000001' AND
-            is_default


Is removing this constraint correct as a part of this PR? I'm not sure how reservation_type / multiple external pools interacts with is_default. Will we let people set a default external service pool?

Good question -- I put this in place recently when I thought we'd reserve pools by linking them to the internal silo. Since we're not doing that anymore, it seemed more confusing than helpful to keep the constraint.

However! I realize I need a data migration to remove those links to the internal silo. I'll add that now.

Added the deletion in beb5523

jgallagher · 2025-10-08T14:50:09Z

nexus/db-queries/tests/output/reserve_external_ip_pool.sql

+  AND reservation_type = $3
+  AND CAST(
+      IF(
+        EXISTS(SELECT 1 FROM ip_pool_resource WHERE ip_pool_id = $4 LIMIT 1),


More thinking out loud - this all looks correct to me, but I'm vaguely worried that our checks to switch between reservation types are so different. If I want to change something away from "for external silos", I have to check the ip_pool_resource table, but if I want to change something away from "for Oxide internal", I have to check the external_ip table. Should rows in external_ip also have corresponding rows in ip_pool_resource?

Should rows in external_ip also have corresponding rows in ip_pool_resource?

I think this is true for addresses used by external silo resources, like instances. So to move a pool away from "for Oxide internal", we couldn't use a check against that table. Not to confuse things further, but we could do that if we went back to linking the pool to the internal silo when we reserve it "for Oxide internal".

…ip-pool-delegation

bnaecker force-pushed the internal-support-for-ip-pool-delegation branch from f3c16d2 to 15bc99a Compare September 27, 2025 15:30

bnaecker force-pushed the internal-support-for-ip-pool-delegation branch from 15bc99a to 44a2d02 Compare September 29, 2025 20:31

bnaecker requested review from david-crespo, jgallagher and smklein September 29, 2025 20:34

bnaecker marked this pull request as draft October 1, 2025 16:34

bnaecker added 3 commits October 2, 2025 21:58

Track IP Pool delegation directly in the ip_pool table

05dfc80

- Add an `is_delegated` column to the `ip_pool` table - Enforce that pools are delegated XOR linkable to customer silos by looking at this table and using conditional update queries.

Typo

b3da59b

Schema migration idempotency fix

34fa464

bnaecker mentioned this pull request Oct 3, 2025

lots of Crucible test failures due to timeouts #8738

Open

bnaecker marked this pull request as ready for review October 3, 2025 18:22

Merge remote-tracking branch 'origin/main' into internal-support-for-…

daa1cee

…ip-pool-delegation

bnaecker changed the title ~~Internal support for linking any number of IP Pools and Silos.~~ Database support for delegating IP Pools for Oxide internal use Oct 3, 2025

jgallagher reviewed Oct 6, 2025

View reviewed changes

bnaecker requested a review from jgallagher October 7, 2025 18:25

bnaecker added 2 commits October 7, 2025 19:31

SQL typo

d59a1ca

More typos

44a947c

jgallagher reviewed Oct 8, 2025

View reviewed changes

bnaecker added 2 commits October 8, 2025 17:02

More review feedback

beb5523

Merge remote-tracking branch 'origin/main' into internal-support-for-…

7a1d0b5

…ip-pool-delegation

bnaecker requested a review from jgallagher October 8, 2025 18:51

bnaecker mentioned this pull request Oct 8, 2025

Failed to download host image in CI: helios / build TUF repo #8519

Open

Database support for delegating IP Pools for Oxide internal use #9103

Are you sure you want to change the base?

Database support for delegating IP Pools for Oxide internal use #9103

Uh oh!

Conversation

bnaecker commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnaecker commented Oct 1, 2025

Uh oh!

david-crespo commented Oct 1, 2025

Uh oh!

bnaecker commented Oct 1, 2025

Uh oh!

bnaecker commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bnaecker commented Sep 26, 2025 •

edited

Loading